Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FE] Typography component #78

Merged

Conversation

wiktoriasalamon
Copy link
Collaborator

No description provided.

@wiktoriasalamon wiktoriasalamon linked an issue Jul 3, 2024 that may be closed by this pull request
2 tasks
className?: string;
}

export type TypographyVariants =
Copy link
Collaborator Author

@wiktoriasalamon wiktoriasalamon Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering splitting variant and font weight to different parameters, but I decided to leave it this way to stay consistent with typography styles from Figma (I only shortened 'Heading' to 'head'). I think it would be easier to use while implementing new views:
For example:
image
Jane Edge has a style named "Headline M/Meddium", so for the Typography component we should use the head-m/medium variant

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I've split button styles into two separate props to reflect Figma. Your approach makes sense to me 👌

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good ideas, let's go with that

className?: string;
}

export type TypographyVariants =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I've split button styles into two separate props to reflect Figma. Your approach makes sense to me 👌

Comment on lines 36 to 40
{small ? (
<Typography variant="body-m/semibold">{title}</Typography>
) : (
<Typography variant="head-s/semibold">{title}</Typography>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe something like that:

Suggested change
{small ? (
<Typography variant="body-m/semibold">{title}</Typography>
) : (
<Typography variant="head-s/semibold">{title}</Typography>
)}
<Typography variant={small ? "body-m/semibold" : "head-s/semibold"}>{title}</Typography>

@wiktoriasalamon wiktoriasalamon merged commit 13e1d8f into develop Jul 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants